Code Review

This is a clean, well-executed feature that solves a real pain point — assertion failures that swallow the original cause. The implementation is backward-compatible, consistently applied across all 22 catch sites, and snapshot tests are updated correctly. Good work overall.

Design Concern: Double-Wrapping in WaitsForAssertion

The most interesting edge case is in WaitsForAssertion.cs:

// lastException is an AssertionException caught from assertion.AssertAsync()
catch (AssertionException ex) { lastException = ex; }

// ...then later:
return AssertionResult.Failed(..., lastException);

// ...which in Assertion.CreateException becomes:
new AssertionException(outerMessage, lastException) // AssertionException wrapping AssertionException

This creates an AssertionException whose InnerException is another AssertionException. The outer message already includes the last error text via ExtractAssertionMessage(lastException), so the inner exception is somewhat redundant and can produce confusing stack traces.

A cleaner approach might be to attach lastException.InnerException instead — preserving the original root cause without the redundant assertion wrapping layer. Alternatively, the WaitsFor case could be treated as a special case where the inner exception isn't threaded (keep null), since the error message is already extracted and embedded in the outer message. Worth deciding intentionally.

API Asymmetry: AssertionResult<T> Missing Public Failed Overload

AssertionResult gains a public Failed(string, Exception?) overload, but AssertionResult<T>.Failed is internal:

// AssertionResult — public ✓
public static AssertionResult Failed(string message, Exception? exception) => new(false, message, exception);

// AssertionResult<T> — internal only, no exception overload
internal static AssertionResult<T> Failed(string message) => new(false, message, default);

The implicit conversion operator correctly propagates the exception, so the common case is handled. But if a custom assertion ever returns AssertionResult<T> directly (rather than implicitly converting from AssertionResult), there's no way to attach an exception. This is a minor gap but worth noting if AssertionResult<T> is part of the extensibility surface.

Minor: FailIf Has No Exception Overload

FailIf(bool condition, string message) is a convenience helper. Since exceptions are typically only available in catch blocks (where you're already constructing the result directly), this is low priority — but worth keeping in mind for consistency if the API evolves further.

What's Working Well

• Backward compatibility is maintained cleanly with the additive overload
• The implicit conversion AssertionResult → AssertionResult<T> correctly threads result.Exception through (line 44 in AssertionResult{T}.cs)
• Assertion.CreateException() applies the inner exception only when present — no null safety issues
• All catch sites consistently updated, including the mock assertions in a separate project
• Public API snapshots updated across all target frameworks

The double-wrapping concern in WaitsForAssertion is the one item I'd recommend addressing before merge, as it's the only case where the attached exception is itself an AssertionException rather than an originating cause.

Follow-up Review

The second commit ("Add context to GroupAssertion error messages") is a nice improvement — the error messages are now more contextual and consistent with how other assertion failures report their location (e.g. failed to get group 'name': ... rather than the bare exception message).

Outstanding: Double-Wrapping in WaitsForAssertion Still Present

My previous concern about WaitsForAssertion has not been addressed. The lastException being threaded through is itself an AssertionException (caught on line 63), so the resulting exception chain becomes:

AssertionException: "assertion did not pass within 1000ms after N attempts. Last error: ..."
  └── InnerException: AssertionException: "expected X but was Y"
                        └── (possibly another inner exception chain)

The outer message already includes the last assertion error text via ExtractAssertionMessage, so the InnerException is a duplicate. More importantly, it's misleading — the InnerException on an exception conventionally means "this exception happened because of the inner one," but here the inner exception is the last of potentially many failures.

Two cleaner options:

Option A — Unwrap to the original cause:

return AssertionResult.Failed(
    $"assertion did not pass within ...", 
    lastException?.InnerException ?? lastException);

This preserves the originating exception (e.g. a NullReferenceException that caused the assertion to fail) without the redundant AssertionException wrapper.

Option B — Skip the inner exception for this case (keep null):

return AssertionResult.Failed(
    $"assertion did not pass within ...");  // no exception — message is self-contained

Since ExtractAssertionMessage already embeds the last error in the message, the inner exception is low-value here and can produce confusing traces.

Option A is probably better if the original inner exception (e.g. the cause of the last AssertionException) is useful for debugging. Option B is simpler and avoids confusion if the inner exception is typically another AssertionException with no originating cause.

Everything else looks good — the GroupAssertion improvement is a nice touch, and the rest of the changes remain solid.

Follow-up Review (commit 3c538ca)

The third commit is a significant expansion — propagating exceptions through the ~110 remaining metadata.Exception guard sites and adding the _passed static readonly cache. Both are good. The caching micro-optimization is the right call here (cold path is now two comparisons rather than a struct allocation).

Outstanding: WaitsForAssertion Double-Wrapping Still Present

My concern from the previous two reviews remains unaddressed. The current state of WaitsForAssertion.cs still passes lastException (an AssertionException) as the inner exception:

// lastException is caught as AssertionException (line 63)
catch (AssertionException ex) { lastException = ex; }

// ...then passed through:
return AssertionResult.Failed(
    $"assertion did not pass within {_timeout.TotalMilliseconds:F0}ms after {attemptCount} attempts. {lastErrorMessage}",
    lastException);  // ← this is itself an AssertionException

This produces a misleading chain:

AssertionException: "assertion did not pass within 1000ms after N attempts. Last error: expected X but was Y"
  └── InnerException: AssertionException: "expected X but was Y"   ← duplicate

The outer message already contains ExtractAssertionMessage(lastException), so the InnerException is pure duplication, and it's semantically wrong — InnerException conventionally means "the root cause", not "the last of N transient failures".

The fix is a one-liner:

// Option A — unwrap to the originating cause (keeps diagnostic value):
return AssertionResult.Failed(
    $"assertion did not pass within ...",
    lastException?.InnerException ?? lastException);

// Option B — omit the inner exception (message is self-contained):
return AssertionResult.Failed(
    $"assertion did not pass within ...");

New Finding: MatchIndexAssertion Inconsistency

The third commit updated MatchIndexAssertion.cs but left an inconsistency in the if (exception != null) block:

if (exception != null)
{
    if (exception is ArgumentOutOfRangeException || exception is InvalidOperationException)
    {
        return Task.FromResult(AssertionResult.Failed(exception.Message));  // exception NOT propagated
    }
    return Task.FromResult(AssertionResult.Failed($"threw {exception.GetType().Name}", exception));  // propagated ✓
}

The first path loses the exception object. Since the goal of this PR is to preserve original exceptions for debugging, this branch should also pass the exception:

return Task.FromResult(AssertionResult.Failed(exception.Message, exception));

These two exceptions (ArgumentOutOfRangeException, InvalidOperationException) represent legitimate error causes — not passing them through is inconsistent with everything else in this PR.

Summary

The WaitsForAssertion issue is the blocking item — it's the only place where the "inner exception" is itself an AssertionException, inverting the intent of the feature.

Code Review: Add AssertionResult.Failed overload that accepts an Exception

The feature itself is well-motivated and the mechanical propagation across 40+ files is correct. A few observations on design and completeness:

1. Two missed propagation sites in StringAssertions.cs

Line 546 (HasLengthAssertion.CheckAsync) still uses the old overload without forwarding the exception:

// Line 546 - exception is in scope but not forwarded
return AssertionResult.Failed($"threw {exception.GetType().Name}");

Line 666 (IsMatchingRegexAssertion.CheckAsync) uses exception.Message but also omits the exception itself:

return Task.FromResult(AssertionResult.Failed(exception.Message));

These two sites were missed in the sweep. The second one also intentionally omits the exception type from the message, which is inconsistent with every other metadata.Exception guard (all of which say "threw {type.Name}").

2. AssertionResult<T> still has no public Failed(string, Exception?) overload

The non-generic AssertionResult gained public static Failed(string, Exception?), but AssertionResult<T> only has:

internal static AssertionResult<T> Failed(string message) => new(false, message, default);

The Exception property is propagated correctly through the implicit conversion operator, but any assertion that directly creates AssertionResult<T>.Failed(...) cannot attach an exception. If AssertionResult<T> is ever used in a catch site (generated assertions via [GenerateAssertion], for instance), there is no ergonomic path. Consider:

internal static AssertionResult<T> Failed(string message, Exception? exception = null)
    => new(false, message, default, exception);

3. FailIf has no exception-carrying overload

FailIf(bool condition, string message) is in the public API but has no FailIf(bool condition, string message, Exception? exception) counterpart. Any caller that wants to conditionally fail with a contextual exception must manually write condition ? Failed(msg, ex) : Passed, breaking the helper's value. The symmetric approach would be:

public static AssertionResult FailIf(bool condition, string message, Exception? exception)
    => condition ? Failed(message, exception) : Passed;

4. WaitsForAssertion unwrapping may lose the inner exception chain

lastException?.InnerException ?? lastException

WaitsForAssertion passes lastException.InnerException to avoid the AssertionException → AssertionException chain. This is reasonable, but if the original assertion itself wrapped a cause (e.g., an EqualityException that itself has an InnerException), the intermediate AssertionException is the richer object: it carries the formatted message the user wrote. Silently unwrapping one level means the inner exception in the final AssertionException is the raw comparison exception with no assertion context.

A safer approach would be to preserve the whole AssertionException as the inner exception and rely on the outer AssertionException's message (already including "Last error: …") to provide context, rather than reaching into its InnerException. If the concern is readability of the exception chain, add a helper property/method to distinguish "assertion failed" wrapping from "original cause" wrapping, rather than unwrapping by convention.

5. The _passed field initialization race for the _passedTask

private static readonly AssertionResult _passed = new(true, string.Empty);
internal static readonly Task<AssertionResult> _passedTask = Task.FromResult(Passed);

_passedTask calls the Passed property (=> _passed) at field-initializer time, which is fine because static field initializers run top-to-bottom. However, the intent of the _passed field is to cache the struct to avoid per-call allocations on the value type. Since AssertionResult is a readonly struct, returning it from a property copies the struct on every call — the cached field helps avoid the allocation of a new instance each time Passed is referenced. This is a good optimization but might benefit from a comment explaining why _passed exists (since the optimization is subtle for a readonly struct returned by value).

6. No tests added for the new Exception propagation

The PR updates public API snapshots and mentions builds pass, but there are no new test cases demonstrating that InnerException is actually populated on a thrown AssertionException. Given the breadth of catch sites changed, at least one integration test covering a representative case (e.g., a collection assertion where the evaluated delegate threw, verifying AssertionException.InnerException != null) would give confidence that the threading actually works end-to-end.

Summary

The foundation is solid and the direction is right. The two missed sites and the AssertionResult<T> / FailIf gaps are the most actionable items before merging.